fix(core): preserve fallback models after failed discovery - #3581
fix(core): preserve fallback models after failed discovery#3581mikemikimike wants to merge 4 commits into
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Independent review at exact head 073a9cba387d22314b3446ca13bd218ff363672c.
APPROVE. The distinction this draws is the right one, and it is drawn in the only place that can carry it.
The bug was that an empty array is truthy, so input.modelSource ?? (liveModels ? 'fetched' : 'fallback') turned "discovery has not succeeded" into "the provider enumerated nothing", and the entire static catalog vanished from the picker while authorizeConnectionModel was still happily admitting those same models. The old comment on that line already claimed to use the raw modelSource — the code did not. This PR makes the code match the comment.
I walked the four reachable input shapes at this head rather than taking the tests' word for it:
models |
modelSource |
inventory |
source |
entries |
|---|---|---|---|---|
undefined |
undefined |
absent |
static_catalog |
fallback catalog |
[] |
undefined (failed/pending discovery) |
absent |
static_catalog |
fallback catalog — the fix |
[] |
'fetched' (authoritative empty) |
live |
provider_api |
empty; saved default annotated not_in_live_list |
| non-empty | either | per source | per source | live list |
The pairing holds in every row: classifyConnectionModelInventory gates on models === undefined || modelSource === undefined → 'absent', so the fallback catalog can never be labelled provider_api. I specifically probed the shape that would break that — models: undefined with an explicit modelSource: 'fetched' — and it resolves to absent/static_catalog, so provenance stays honest there too.
Worth noting for the record: the defaulted local modelSource also feeds the staleness/annotation paths at model-catalog.ts:207,219,236, so models: [] with no source now reads fallback there where it previously read fetched. That is the same correction, applied consistently, not a side effect.
The two new tests pin behaviour rather than call shape, and the second one is the load-bearing half — without it, "always fall back on empty" would pass just as well and would silently resurrect models a provider has genuinely retired.
Verification: exact-head test is completed/success. Dependency audit is absent on this head by design — it is path-filtered to package.json / lockfile / audit scripts, neither of which this PR touches.
b04169e to
b2a895d
Compare
b2a895d to
cb37702
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed on exact head cb3770233099d5ca454effed4ceb3b930964b895. No P0–P2 in the change itself. One pre-existing [P3] inline. Not approving yet, for a gating reason rather than a code reason — details below.
The fix is correct, and for the right reason. Both original sites treated an empty array as "the provider authoritatively answered: none":
- the
modelSourceinferenceliveModels ? 'fetched' : 'fallback'—[]is truthy, so "never looked" was scored as "looked and found nothing"; rawModels = liveModels ?? fallback—??only guardsundefined, so[]displaced the entire static fallback catalog.
Introducing the "is there an authoritative source" dimension separates the three states cleanly: undefined (never queried) → fallback; [] without an explicit modelSource: 'fetched' (failed or pending discovery) → fallback, which is the bug being fixed; [] with explicit 'fetched' (provider genuinely has none) → stays empty.
I also confirmed classifyConnectionModelInventory still receives the raw input.modelSource rather than the inferred value — that is what the original comment there was protecting, and the change does not lose it.
The three new tests pin both branches of the new condition plus one public-path case, rather than only asserting shape.
On the red check — it is not this PR's fault, but it still blocks. The failing test comes from the branch being cut from an older main that contained a broken test: at the merge base, goal-services-adapter.test.ts:64/70/76 pass a type: 'sessions_changed' field that SessionChangedEvent does not have (its fields are reason/sessionId/modelId/turnId/ts), so that base does not compile. Current main has already dropped those type fields. This PR touches only model-catalog.ts and its test — verified via git diff --name-only merge-base..head.
So: no finding recorded for it, but an unrelated red is still not green, and the approval bar is terminal-green expected checks on the exact head. Merging current main into the branch should clear it, and I will re-check the terminal state after that.
| liveModels !== undefined && (liveModels.length > 0 || modelSource === 'fetched') | ||
| ? liveModels | ||
| : (input.fallbackModels ?? []).map((id) => ({ | ||
| id, |
There was a problem hiding this comment.
[P3] Pre-existing, not introduced here (anchored to this hunk because the affected line at :270 is unchanged and cannot take an inline comment) — but it sits directly adjacent to what this PR fixes, so it is worth naming while the area is being touched.
A connection that succeeded at discovery (modelSource === 'fetched') but whose models were all later quarantined ends up here with [] after the filter, still carrying 'fetched'. That takes the "authoritatively empty" branch, so the picker renders empty and no fallback catalog is offered — even though the static fallback may contain usable models.
Trigger → path → outcome: a connection works normally, every one of its model ids later lands in brokenModelIds, the user opens the model picker and sees nothing.
From the user's side this is indistinguishable from the failed-discovery case this PR is fixing, but only the latter is covered. Behavior is unchanged from before this PR (the old code also reduced to rawModels = liveModels = [] here), so this is not a regression — hence P3 rather than a blocker.
Smallest fix: check for emptiness after filtering — if the pre-filter list was non-empty and the post-filter list is empty, treat it as having no usable discovery result and fall back. A production-seam test would set models such that every id hits brokenModelIds with modelSource: 'fetched', and assert fallback catalog entries are still returned.
2 participants
Summary
Fixes the independent fallback-model half of #3320.
When model discovery has failed or has not run yet, the catalog can contain an empty
models: []array without a successfulmodelSource. The picker treated that empty array as authoritative and hid the static fallback catalog, even though the connection could still use the configured fallback model.This change:
fallback, rather thanfetched, for an empty model array when no source is persisted;The URL-normalization question in #3320 is intentionally out of scope.
Validation
npm --workspace @maka/core run typechecknpm --workspace @maka/core run buildnpm --workspace @maka/core run test:dist— 638 passednpx biome check packages/core/src/model-catalog.ts packages/core/src/__tests__/model-catalog.test.tsnpx biome lint packages/core/src/model-catalog.ts packages/core/src/__tests__/model-catalog.test.tsgit diff --check